Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(time-picker): Added min and max props #590

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

jason-evans-genesys
Copy link
Collaborator

@jason-evans-genesys jason-evans-genesys commented Sep 6, 2024

✅ Closes: COMUI-1736

Added optional min and max props for the time-picker component.

@jason-evans-genesys jason-evans-genesys self-assigned this Sep 6, 2024
Copy link

github-actions bot commented Sep 6, 2024

@jason-evans-genesys jason-evans-genesys changed the title feat(time-picker): Added min and max props (IN PROGRESS) feat(time-picker): Added min and max props (WIP) Sep 6, 2024
@jason-evans-genesys jason-evans-genesys changed the title feat(time-picker): Added min and max props (WIP) feat(time-picker): Added min and max props Sep 10, 2024
@321gillian
Copy link
Collaborator

LGTM but I think worth bringing up the questions you have with the team

@gavin-everett-genesys
Copy link
Collaborator

I was just reading up on how the start and end would work if you were crossing over midnight for example if you have a min of 22:00 and an end of 04:00. Apparently for the native timepicker it wraps around midnight . So it would,

  • The valid time range would start at 22:00 (10:00 PM) and continue until 23:59 (11:59 PM).
  • Then, it wraps around midnight, continuing from 00:00 (12:00 AM) to 04:00 (4:00 AM).

Still uncertain how it works in terms of wrapping around midnight but I think you could potentially create a validation function .I think it might be possible to convert the start and end times into a comparable format like seconds from midnight. You could then check If min is less than max check if the time value falls between min and max so eg

value = 01:00 , start = 22:00 , end = 04:00

if ( 22:00 < 04:00 ) then
// no wrap around
      return 1:00 >= 22:00 && 1:00 <= 04:00 
else
//wrap around
    return 1:00 >= 22:00 || 1:00 <= 04:00

so this should return true. This is just a rough idea of what I was thinking about but not sure if we want to be thinking about the crossover.

@jason-evans-genesys
Copy link
Collaborator Author

I was just reading up on how the start and end would work if you were crossing over midnight for example if you have a min of 22:00 and an end of 04:00. Apparently for the native timepicker it wraps around midnight . So it would,

* The valid time range would start at 22:00 (10:00 PM) and continue until 23:59 (11:59 PM).

* Then, it wraps around midnight, continuing from 00:00 (12:00 AM) to 04:00 (4:00 AM).

Still uncertain how it works in terms of wrapping around midnight but I think you could potentially create a validation function .I think it might be possible to convert the start and end times into a comparable format like seconds from midnight. You could then check If min is less than max check if the time value falls between min and max so eg

value = 01:00 , start = 22:00 , end = 04:00

if ( 22:00 < 04:00 ) then
// no wrap around
      return 1:00 >= 22:00 && 1:00 <= 04:00 
else
//wrap around
    return 1:00 >= 22:00 || 1:00 <= 04:00

so this should return true. This is just a rough idea of what I was thinking about but not sure if we want to be thinking about the crossover.

@gavin-everett-genesys This is ready for review again. Thanks!

@katie-bobbe-genesys
Copy link
Collaborator

Should min and max work with the 12 hour format as well? Pressing the AM/PM button allows for selecting "out of bounds" times in 12 hour format mode

@jason-evans-genesys
Copy link
Collaborator Author

jason-evans-genesys commented Oct 10, 2024

Should min and max work with the 12 hour format as well? Pressing the AM/PM button allows for selecting "out of bounds" times in 12 hour format mode

@daragh-king-genesys I think you had mentioned we should not do the min and max for the 12 hour format but correct me if I'm wrong.

@katie-bobbe-genesys
Copy link
Collaborator

Should min and max work with the 12 hour format as well? Pressing the AM/PM button allows for selecting "out of bounds" times in 12 hour format mode

@daragh-king-genesys I think you had mentioned we should not do the min and max for the 12 hour format but correct me if I'm wrong.

If it is the case that 12 hour format is not supported I would suggest the following

  1. do a check like if format is not 12hour and there is min / max prop then use the min max prop logic, otherwise ignore it
  2. perhaps consider logWarn if the format is 12hr and there is min/max prop added

@@ -102,6 +108,10 @@ export class GuxTimePicker {

this.i18n = await buildI18nForComponent(this.root, translationResources);
this.clockType = this.clockType || getLocaleClockType(this.root);

if (this.clockType == '12h' && (this.min || this.max)) {
console.error('clock type must be "24h" when using min/max props');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katie-bobbe-genesys I had this code here already to check if the clock type is 12h but I used a console error. Should I keep this code but change to a console warn? I'll also do an if check like you mentioned to ignore the min/max logic in a certain scenario.

return acc.concat(
minuteOptions.map(
minuteOption => `${hourOption}:${minuteOption}`
) as GuxISOHourMinute[]
);
}, [] as GuxISOHourMinute[]);

return clockType === '24h'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katie-bobbe-genesys this is where I check if the clocktype is 24h before applying the min/max check.

✅ Closes: COMUI-1736

feat(time-picker): Fixed up min/max logic

✅ Closes: COMUI-1736

feat(time-picker): Fixed up min/max logic to make it more functional

✅ Closes: COMUI-1736

test(time-picker): Added min and max tests

✅ Closes: COMUI-1736

test(time-picker): Fixed 12h test for min and max

✅ Closes: COMUI-1736

chore(time-picker): General refactoring

✅ Closes: COMUI-1736

test(time-picker): Fixed min/max tests

✅ Closes: COMUI-1736

chore(time-picker): Refactoring changes

✅ Closes: COMUI-1736

chore(time-picker): Fixed comment to be more descriptive

✅ Closes: COMUI-1736

test(time-picker): Fixed test title

✅ Closes: COMUI-1736

chore(time-picker): PR feedback

✅ Closes: COMUI-1736

chore(time-picker): Fixed hour conversion logic

✅ Closes: COMUI-1736

chore(time-picker): Variable name change

✅ Closes: COMUI-1736

chore(time-picker): Removed unneeded 12h clock type test

✅ Closes: COMUI-1736

chore(time-picker): Added tests and wrapped around boundary logic

✅ Closes: COMUI-1736

chore(time-picker): Conditional check before applying boundaries

✅ Closes: COMUI-1736

chore(time-picker): Fixed boundary logic to be inclusive of min and max

✅ Closes: COMUI-1736
@jason-evans-genesys jason-evans-genesys merged commit 895a4f2 into main Oct 15, 2024
3 checks passed
@jason-evans-genesys jason-evans-genesys deleted the feature/COMUI-1736 branch October 15, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants